Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bump up Yorkie to v0.5.4 #176

Merged
merged 1 commit into from
Oct 28, 2024
Merged

Bump up Yorkie to v0.5.4 #176

merged 1 commit into from
Oct 28, 2024

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Oct 28, 2024

What this PR does / why we need it?

Bump up Yorkie to v0.5.4

Any background context you want to provide?

What are the relevant tickets?

Fixes #

Checklist

  • Added relevant tests or not required
  • Didn't break anything

Summary by CodeRabbit

Release Notes

  • New Features

    • Updated the environment variable VITE_JS_SDK_VERSION to version 0.5.4.
    • Introduced a new VersionVector message for enhanced versioning in document management.
    • Added new message classes for user authentication and project management in the Yorkie API.
  • Improvements

    • Enhanced type safety by changing previousSeq parameter types from string to bigint in relevant functions.
    • Updated TypeScript configuration to target ES2020 for improved language features.
  • Bug Fixes

    • Removed JavaScript-specific type hints from protocol buffer message definitions for cleaner data handling.
  • Chores

    • Cleaned up deprecated files related to user account and project management in the Yorkie API.

@chacha912 chacha912 requested a review from hackerwins October 28, 2024 08:44
Copy link

coderabbitai bot commented Oct 28, 2024

Walkthrough

The pull request includes updates to various files, primarily focusing on version increments and type modifications. The environment variable VITE_JS_SDK_VERSION has been updated from 0.5.1 to 0.5.4. The buf.gen.yaml file has been modified for TypeScript-centric output. The package.json file reflects version updates for the project and its dependencies. Several TypeScript files have undergone changes to improve type safety, particularly concerning the handling of bigint types instead of string. Additionally, multiple files related to the Yorkie API have been added or removed, reflecting structural changes in the API.

Changes

File Change Summary
.env Updated VITE_JS_SDK_VERSION from 0.5.1 to 0.5.4.
buf.gen.yaml Updated target option from js+dts to ts for es and connect-es plugins; replaced js_import_style with import_extension=none.
package.json Updated project version from 0.5.1 to 0.5.4 and yorkie-js-sdk dependency from ^0.5.1 to ^0.5.4.
src/api/index.ts Modified previousSeq parameter type in listDocumentHistories from string to bigint; updated logic for handling serverSeq and applySnapshot method with VersionVector.
src/api/types.ts Changed serverSeq property type in DocumentHistory from string to bigint.
src/api/yorkie/v1/admin.proto Removed jstype = JS_STRING from server_seq, lamport, and previous_seq fields; added version_vector field to GetSnapshotMetaResponse.
src/api/yorkie/v1/admin_connect.d.ts Removed AdminService TypeScript declarations.
src/api/yorkie/v1/admin_connect.ts Updated import path for admin_pb and changed export of AdminService to use constant assertion.
src/api/yorkie/v1/admin_pb.d.ts Removed various user account and project management class definitions.
src/api/yorkie/v1/admin_pb.js Removed protocol buffer message definitions for user account and project management.
src/api/yorkie/v1/admin_pb.ts Added new TypeScript definitions for user account and project management message classes generated from protobuf.
src/api/yorkie/v1/resources.proto Added new VersionVector message; updated ChangePack, ChangeID, and Operation messages with new fields and removed jstype options.
src/api/yorkie/v1/resources_pb.d.ts Removed various enumerations and class definitions related to data structures in the Yorkie API.
src/api/yorkie/v1/resources_pb.js Removed protocol buffer definitions for various message types and enumerations.
src/api/yorkie/v1/resources_pb.ts Added new TypeScript definitions for various message classes and enumerations generated from protobuf.
src/features/documents/documentsSlice.ts Updated previousSeq parameter type in listDocumentHistoriesAsync from string to bigint.
tsconfig.json Updated compilerOptions.target from "es5" to "ES2020".

Possibly related PRs

  • Bump up Yorkie to v0.5.1 #174: The changes in this PR involve updating the VITE_JS_SDK_VERSION environment variable and the yorkie-js-sdk dependency version in package.json, which are directly related to the version update of VITE_JS_SDK_VERSION from 0.5.1 to 0.5.4 in the main PR.

Suggested reviewers

  • hackerwins

Poem

In the meadow, changes bloom,
Versions rise, dispelling gloom.
With bigint now, we leap and bound,
In code's embrace, new joys are found.
Hops of progress, swift and bright,
A rabbit's cheer for code's delight! 🐇✨


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

@hackerwins hackerwins merged commit 2cf3bf5 into main Oct 28, 2024
2 checks passed
@hackerwins hackerwins deleted the v0.5.4 branch October 28, 2024 08:48
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (6)
tsconfig.json (1)

3-3: Consider documenting minimum browser requirements.

Since ES2020 features are now being targeted, it would be helpful to document the minimum browser versions required for the application.

Add a section in your README.md or documentation about browser compatibility:

+ ## Browser Compatibility
+ 
+ This application requires browsers with ES2020 support:
+ - Chrome >= 84
+ - Firefox >= 79
+ - Safari >= 14
+ - Edge >= 84
src/api/yorkie/v1/admin.proto (2)

164-165: Architectural enhancement with version vector support.

Two significant changes here:

  1. Removal of [jstype = JS_STRING] from lamport field aligns with the bigint handling improvements
  2. Addition of version_vector field enhances version tracking capabilities, which is a positive architectural improvement

The introduction of VersionVector suggests a more sophisticated version tracking mechanism. Ensure that:

  • Client code properly handles the new version vector field
  • Documentation is updated to reflect this enhancement
  • Migration guides are provided if this is a breaking change

Line range hint 1-196: Overall architectural improvements in v0.5.4.

The changes in this proto file reflect two significant improvements:

  1. More robust type handling for int64 fields by removing string representations
  2. Enhanced version tracking through the introduction of version vectors

These changes align well with the upgrade to Yorkie v0.5.4 and should improve type safety and version management capabilities.

Consider:

  • Updating API documentation to reflect these changes
  • Providing migration guides if client code needs updates
  • Adding examples of how to properly use the new version vector feature
src/api/yorkie/v1/resources.proto (2)

49-49: LGTM: Well-structured version vector implementation.

The addition of VersionVector and its integration into ChangePack and ChangeID messages follows best practices for distributed systems. This enhancement will improve causality tracking and conflict resolution capabilities.

This change strengthens Yorkie's distributed consistency model by providing better version tracking at both the change pack and individual change levels.

Also applies to: 64-68


145-150: LGTM: Well-structured ArraySet operation.

The new ArraySet operation follows the established pattern of other operations and includes all necessary timing fields. This addition enhances array manipulation capabilities.

Consider adding a comment above the ArraySet message to document its purpose and usage, similar to how the Select operation is documented.

+  // ArraySet operation allows direct value setting in arrays, providing an alternative
+  // to the existing Add/Remove operations for array manipulation.
   message ArraySet {
     TimeTicket parent_created_at = 1;
     TimeTicket created_at = 2;
     JSONElementSimple value = 3;
     TimeTicket executed_at = 4;
   }

Also applies to: 163-163

src/api/yorkie/v1/admin_pb.ts (1)

19-19: Consider removing @ts-nocheck to enable type checking

The directive @ts-nocheck disables TypeScript type checking for this file. Enabling type checking could help catch potential type errors in the generated code.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 533e0fb and fcac0d1.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (17)
  • .env (1 hunks)
  • buf.gen.yaml (1 hunks)
  • package.json (2 hunks)
  • src/api/index.ts (3 hunks)
  • src/api/types.ts (1 hunks)
  • src/api/yorkie/v1/admin.proto (2 hunks)
  • src/api/yorkie/v1/admin_connect.d.ts (0 hunks)
  • src/api/yorkie/v1/admin_connect.ts (2 hunks)
  • src/api/yorkie/v1/admin_pb.d.ts (0 hunks)
  • src/api/yorkie/v1/admin_pb.js (0 hunks)
  • src/api/yorkie/v1/admin_pb.ts (1 hunks)
  • src/api/yorkie/v1/resources.proto (6 hunks)
  • src/api/yorkie/v1/resources_pb.d.ts (0 hunks)
  • src/api/yorkie/v1/resources_pb.js (0 hunks)
  • src/api/yorkie/v1/resources_pb.ts (1 hunks)
  • src/features/documents/documentsSlice.ts (2 hunks)
  • tsconfig.json (1 hunks)
💤 Files with no reviewable changes (5)
  • src/api/yorkie/v1/admin_connect.d.ts
  • src/api/yorkie/v1/admin_pb.d.ts
  • src/api/yorkie/v1/admin_pb.js
  • src/api/yorkie/v1/resources_pb.d.ts
  • src/api/yorkie/v1/resources_pb.js
✅ Files skipped from review due to trivial changes (1)
  • .env
🔇 Additional comments (21)
buf.gen.yaml (2)

6-6: LGTM: TypeScript-native code generation is a good choice.

Switching from js+dts to ts target is a positive change that:

  • Generates TypeScript files directly instead of JavaScript + declaration files
  • Reduces build complexity by eliminating intermediate JavaScript generation
  • Better aligns with modern TypeScript development practices

Also applies to: 11-11


7-7: LGTM: Import extension configuration improves module compatibility.

Adding import_extension=none is beneficial as it:

  • Provides better compatibility with different module resolution strategies
  • Aligns with modern ES module patterns
  • Helps avoid potential import-related issues in different environments

Also applies to: 12-12

tsconfig.json (1)

3-3: Verify browser compatibility requirements.

The upgrade from ES5 to ES2020 aligns well with the Yorkie v0.5.4 upgrade, especially for BigInt support. However, this change could affect browser compatibility.

Run this script to check if there's any documented browser compatibility requirements:

✅ Verification successful

ES2020 target is appropriate for the codebase's browser requirements

The change from ES5 to ES2020 is well-aligned with both the codebase and browser compatibility requirements:

  • The browserslist configuration shows modern browser support:
    • Production: >0.2%, not dead, not op_mini all
    • Development: Latest Chrome and Firefox versions
  • Multiple files already use ES2020 features (optional chaining, etc.)
  • The configuration aligns with Yorkie v0.5.4's requirements for BigInt support
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for browser compatibility documentation
echo "Checking README and package.json for browser requirements..."
rg -i "browser|compatibility|support" README.md package.json

Length of output: 226


Script:

#!/bin/bash
# Check browserslist configuration
echo "Checking browserslist configuration in package.json..."
cat package.json | jq -r '.browserslist'

# Check for any browser-related configurations in other common config files
echo -e "\nChecking for browser configurations in other files..."
fd -e js -e json -e yaml -e yml -H -E node_modules -x grep -l -i "browser" {}

# Look for any ES2020 specific features in the codebase
echo -e "\nChecking usage of ES2020 features..."
rg "BigInt|?.|\?\?|import\(|Promise\.allSettled" -l

Length of output: 854


Script:

#!/bin/bash
# Fix the regex pattern to properly escape special characters and check for ES2020 features
echo "Checking usage of ES2020 features..."
rg "BigInt|\?\.|\?\?|import\(|Promise\.allSettled" -l

# Check for any type changes related to BigInt
echo -e "\nChecking for BigInt type usage..."
ast-grep --pattern 'type $_: bigint'

Length of output: 685

src/api/types.ts (1)

27-27: LGTM! Type change improves precision handling.

The change from string to bigint for serverSeq is a good improvement as it:

  • Prevents potential precision loss
  • Provides proper type safety for sequence numbers
  • Aligns with the SDK upgrade to v0.5.4

Let's verify the impact on consumers of this type:

✅ Verification successful

Let me analyze the results and generate a final response based on the findings.

Type change to bigint is properly handled across the codebase

The verification shows that:

  • All serverSeq usages are already handling bigint values correctly
  • The SDK's protobuf definitions use int64 which aligns with the bigint type
  • Key usage in api/index.ts shows proper bigint operations (e.g., serverSeq - 1n)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Find all usages of DocumentHistory type to ensure consumers handle bigint properly

# Search for direct DocumentHistory type usage
echo "=== Direct DocumentHistory usage ==="
rg -l "DocumentHistory" --type ts

# Search for serverSeq property access
echo -e "\n=== serverSeq property access patterns ==="
rg "serverSeq" --type ts -A 2 -B 2

Length of output: 1953

package.json (2)

45-45: Verify compatibility with updated yorkie-js-sdk.

The dependency update to ^0.5.4 allows for compatible minor version updates. While the PR indicates no breaking changes, it's important to ensure comprehensive testing, especially given the TypeScript changes mentioned in the summary.

Let's check for potential breaking changes:

#!/bin/bash
# Description: Look for potential breaking changes in TypeScript usage
# Expected: Identify any bigint type changes that might need attention

# Search for bigint type usage that might need updates
echo "Checking for bigint type usage:"
rg "type.*string" --type ts -B2 -A2

# Look for yorkie-js-sdk imports to verify usage
echo -e "\nChecking yorkie-js-sdk usage:"
rg "from ['\"]yorkie-js-sdk['\"]" --type ts -B1 -A1

3-3: Version bump aligns with PR objective.

The project version update to 0.5.4 matches the intended Yorkie upgrade.

Let's verify the version consistency across the project:

✅ Verification successful

Version update is consistent across the project

The version bump to 0.5.4 is properly reflected in:

  • package.json (project version)
  • package-lock.json (yorkie-js-sdk dependency)
  • All imports and usages of yorkie-js-sdk are clean without version-specific code
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check version consistency across project files
# Expected: All version references should be 0.5.4

# Check .env file for VITE_JS_SDK_VERSION
echo "Checking .env file:"
rg "VITE_JS_SDK_VERSION.*0\.5\." .env || echo "Version not found in .env"

# Check for any other version references
echo -e "\nChecking other version references:"
rg "0\.5\.[0-9]" --type-not json

Length of output: 1797


Script:

#!/bin/bash
# Let's check for version references in environment files and configuration
echo "Checking environment files:"
fd -t f "\.env" -x cat {}

echo -e "\nChecking for version in vite config:"
fd -t f "vite\.config" -x cat {}

echo -e "\nChecking package-lock.json for yorkie version:"
rg "yorkie-js-sdk" package-lock.json -C 2

Length of output: 1430

src/api/yorkie/v1/admin.proto (2)

182-182: Consistent type handling change.

The removal of [jstype = JS_STRING] from previous_seq maintains consistency with other int64 field changes.

This change aligns with the overall improvement in type handling across the codebase.


159-159: Verify the impact of removing jstype option.

The removal of [jstype = JS_STRING] from the server_seq field indicates a shift in how int64 values are handled. This change likely improves type safety by using native bigint instead of string representations.

✅ Verification successful

Removal of jstype option is consistent with codebase changes

The verification shows that:

  • No remaining jstype = JS_STRING options exist in any proto files
  • All int64 fields including server_seq are consistently defined without jstype options
  • Generated TypeScript code (*_pb.ts files) properly handles these fields as ScalarType.INT64

This change is part of a consistent update across the codebase to handle int64 values uniformly without string type conversion.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for any remaining jstype = JS_STRING options in proto files
# and verify consistent handling of int64 fields
rg --type proto "jstype = JS_STRING" ./src/api

# Check for related type handling in TypeScript files
rg -t ts "server_seq.*string" ./src

Length of output: 116


Script:

#!/bin/bash
# Search for jstype options in .proto files without relying on file type
rg "jstype = JS_STRING" $(fd -e proto)

# Search for all int64 field definitions in .proto files
rg "int64.*=.*;" $(fd -e proto)

# Search for server_seq usage in all files
rg "server_seq" --type-not proto

Length of output: 719


Script:

#!/bin/bash
# Search for server_seq usage in all files without type filter
rg "server_seq" --type all

# Search for any remaining jstype options in all files
rg "jstype.*=.*JS_STRING"

# Look for TypeScript type definitions or interfaces related to these protos
rg -l "interface.*SnapshotMeta|type.*SnapshotMeta"

Length of output: 893

src/api/yorkie/v1/admin_connect.ts (3)

16-16: LGTM: Auto-generated file header

This is an auto-generated file using protoc-gen-connect-es v1.4.0, which is consistent with the Yorkie version bump.


177-177: Type safety improvement with const assertion

The addition of as const assertion improves type safety by making the AdminService object and its properties read-only at compile time. This is a positive change that prevents accidental modifications to the service definition.


Line range hint 1-177: Verify the generated code matches Yorkie v0.5.4 specifications

Since this is an auto-generated file, we should verify that it matches the expected output for Yorkie v0.5.4.

✅ Verification successful

Let me gather more information about the generated code and its specifications.


Let me verify one more aspect to ensure the generated code matches the proto specification.


Generated code matches Yorkie v0.5.4 specifications

The verification confirms that:

  • The generated code perfectly matches the proto file specification with all 16 service methods correctly implemented
  • All request/response message types are properly imported
  • The code is generated for Yorkie v0.5.4 as specified in package.json
  • The service interface maintains the exact structure defined in the proto file
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the Yorkie version and related configurations

# Check package.json for Yorkie version
echo "Checking Yorkie version in package.json:"
jq -r '.dependencies["yorkie-js-sdk"]' package.json

# Check environment variables for SDK version
echo "Checking SDK version in environment files:"
rg "VITE_JS_SDK_VERSION" -A 1

Length of output: 772


Script:

#!/bin/bash
# Check the proto file that generated this code
echo "Checking the original proto file:"
cat src/api/yorkie/v1/admin.proto

# Check the protoc-gen-connect-es version used
echo -e "\nChecking protoc-gen-connect-es version in package.json:"
jq -r '.devDependencies["@bufbuild/protoc-gen-connect-es"]' package.json

# Check if there are any other generated files with similar patterns
echo -e "\nChecking for other generated files:"
rg "@generated by protoc-gen-connect-es" --files-with-matches

Length of output: 5196


Script:

#!/bin/bash
# Compare the service methods in proto and generated code
echo "Comparing service methods:"

# Extract methods from proto file
echo "Methods in proto file:"
grep "rpc" src/api/yorkie/v1/admin.proto | awk -F'[()]' '{print $1}' | awk '{print $2}'

# Extract methods from generated code
echo -e "\nMethods in generated code:"
grep "name:" src/api/yorkie/v1/admin_connect.ts | awk -F'"' '{print $2}'

# Check the imports to ensure all message types are properly imported
echo -e "\nChecking imports in generated code:"
grep "import {.*} from" src/api/yorkie/v1/admin_connect.ts

Length of output: 1709

src/api/index.ts (4)

17-17: LGTM: Import changes align with SDK upgrade.

The addition of VersionVector import is consistent with the Yorkie SDK v0.5.4 upgrade and its usage in the code.


157-161: LGTM: Improved sequence number handling with bigint.

The change to use native bigint arithmetic (serverSeq - 1n) is more efficient and type-safe compared to string manipulation.


165-165: Verify the empty VersionVector initialization.

While the addition of VersionVector is required by the SDK upgrade, please verify if initializing it with an empty Map is the intended behavior for snapshot application.

✅ Verification successful

Let me gather more context about the VersionVector usage, particularly around snapshot application.


Let me search specifically for the document snapshot application logic to understand the context better.


Based on the gathered context, I can now provide the final response:

Empty VersionVector initialization is correct for snapshot application

The empty VersionVector initialization is the intended behavior here since:

  1. The snapshot is being applied at a specific sequence point obtained from the server
  2. The version vector is initialized fresh as this is a snapshot application, not an incremental change
  3. The actual version information comes from the snapshotMeta.snapshot parameter
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other VersionVector usage patterns in the codebase
ast-grep --pattern 'new VersionVector($$$)'

Length of output: 452


Script:

#!/bin/bash
# Search for applySnapshot implementations and usages
ast-grep --pattern 'applySnapshot($$$)'

# Look for VersionVector related code
rg "VersionVector" -A 3

Length of output: 6031


Script:

#!/bin/bash
# Search for document snapshot related code
rg "applySnapshot" -B 3 -A 3

# Search for snapshot meta related code
rg "snapshotMeta" -B 3 -A 3

Length of output: 1083


143-146: Verify the impact of type change from string to bigint.

The parameter type change from string to bigint is appropriate for sequence numbers, but we should verify that all callers have been updated accordingly.

✅ Verification successful

Let me gather more information to ensure we have a complete view of the type change impact.


Let me try one final verification approach to find the usage patterns.


Type change from string to bigint has been properly implemented

The verification shows the type change is consistently handled:

  • The API function in src/api/index.ts declares previousSeq as bigint
  • The Redux thunk in src/features/documents/documentsSlice.ts correctly:
    • Declares previousSeq?: bigint in its parameters
    • Uses a bigint literal 0n as default value
    • Passes the bigint value to the API function
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for calls to listDocumentHistories to ensure they use bigint
rg -l "listDocumentHistories.*previousSeq.*string" || echo "No string type usage found"

# Search for any remaining string to bigint conversions that might need updating
ast-grep --pattern 'previousSeq: string'

Length of output: 158


Script:

#!/bin/bash
# Search for all usages of listDocumentHistories to see how previousSeq is passed
rg "listDocumentHistories.*previousSeq" -A 2

# Search for any bigint type declarations related to previousSeq
ast-grep --pattern 'previousSeq: bigint'

# Look for any string to bigint conversions that might be related
rg "BigInt\(.*previousSeq.*\)"

Length of output: 122


Script:

#!/bin/bash
# Search for function declarations and calls with broader context
rg -A 5 -B 5 "listDocumentHistories"

# Find any files that might contain the interface or type definitions
fd -e ts -e tsx -x grep -l "DocumentHistory"

# Look for any sequence-related type conversions more broadly
rg "BigInt\(" -A 2

Length of output: 4549

src/features/documents/documentsSlice.ts (3)

127-127: LGTM: Type change from string to bigint improves type safety

The change from string to bigint for previousSeq is a good improvement as it provides better type safety and precision for sequence numbers.


133-133: LGTM: Appropriate use of bigint literal for default value

The default value 0n correctly uses the bigint literal syntax and maintains type consistency with the parameter type.


Line range hint 127-145: Verify consistent bigint usage across callers

Let's verify that all callers of listDocumentHistoriesAsync have been updated to handle the new bigint type for previousSeq.

✅ Verification successful

Based on the results from our verification scripts, I can now provide a final response:

bigint type change appears to be properly contained

The changes to previousSeq type are well-encapsulated within the documentsSlice.ts and api/index.ts files, where:

  • The type is properly defined as bigint in the interface
  • Default value is correctly set as 0n (bigint literal)
  • The value is properly converted to string when needed via String(previousSeq)
  • The API layer correctly handles the bigint type

No direct usage of listDocumentHistoriesAsync was found in UI components, suggesting the pagination logic is likely handled through Redux state management, which properly processes the values without exposing the implementation details.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for calls to listDocumentHistoriesAsync
rg "listDocumentHistoriesAsync\(" -A 3

# Search for any remaining string-typed previousSeq references
ast-grep --pattern 'previousSeq: string'

Length of output: 83


Script:

#!/bin/bash
# Search for calls to listDocumentHistoriesAsync with more context and case-insensitive
rg -i "listDocumentHistoriesAsync" -B 2 -A 5

# Search for type definitions or interfaces containing previousSeq
ast-grep --pattern 'interface $_ {
  $$$
  previousSeq: $_
  $$$
}'

# Search for any direct usage of previousSeq
rg "previousSeq" -B 2 -A 2

Length of output: 4041


Script:

#!/bin/bash
# Search for components or hooks that might be using the document history functionality
rg -l "useAppSelector.*history" -g "*.tsx" -g "*.ts"

# Search for dispatch calls with listDocumentHistoriesAsync
rg "dispatch\(.*listDocumentHistoriesAsync" -B 2 -A 2

# Look for any UI components handling previousSeq
rg "previousSeq" -g "*.tsx" -B 2 -A 2

Length of output: 148

src/api/yorkie/v1/resources.proto (1)

61-62: Verify JavaScript client compatibility.

The removal of jstype = JS_STRING option for int64 fields could affect JavaScript clients. This change means these fields will now be handled as bigints in JavaScript instead of strings.

Let's verify the impact:

Also applies to: 341-341, 352-352

✅ Verification successful

Let me check the content of these files to understand how these fields are being used.


Based on the search results, I can now generate the final response:

The int64 fields are already using proper bigint handling

The generated TypeScript code shows that server_seq and lamport fields are properly handled using protoInt64.zero initialization and ScalarType.INT64 type definitions. The removal of jstype = JS_STRING aligns with the current implementation as:

  • All int64 fields are initialized with protoInt64.zero
  • The field definitions use proper ScalarType.INT64 type
  • No string-based conversions are being used in the codebase

This indicates the JavaScript/TypeScript client is already prepared for bigint handling, making this change safe.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any JavaScript files that might be affected by the int64 type change

# Look for files accessing these fields
rg -l "server_seq|lamport" --type ts --type js

# Look for any type definitions or conversions
rg -l "(?i)(string.*int64|int64.*string)" --type ts

Length of output: 200


Script:

#!/bin/bash
# Check the content of affected files
echo "=== resources_pb.ts ==="
rg -A 5 -B 5 "server_seq|lamport" src/api/yorkie/v1/resources_pb.ts

echo "=== admin_pb.ts ==="
rg -A 5 -B 5 "server_seq|lamport" src/api/yorkie/v1/admin_pb.ts

# Check for any type conversion patterns
echo "=== Type conversions ==="
rg "toString\(\)|parseInt|Number|BigInt" src/api/yorkie/v1/resources_pb.ts

Length of output: 4719

src/api/yorkie/v1/admin_pb.ts (1)

21-1346: Approved: Generated code aligns with protobuf definitions

The generated classes and methods accurately represent the protobuf messages and are correctly implemented according to the @bufbuild/protobuf library conventions.

src/api/yorkie/v1/resources_pb.ts (1)

233-235: Ensure proper handling of the new versionVector field in ChangePack and ChangeID.

The addition of the versionVector field to ChangePack and ChangeID may require updates in serialization, deserialization, and processing logic throughout the codebase. Please verify that all relevant methods and functions correctly handle this new field to ensure consistent version tracking and synchronization.

Run the following script to identify code that may need to be updated:

Also applies to: 351-353

✅ Verification successful

Based on the search results, I can now provide a final response:

The versionVector field is properly handled in the protobuf-generated code

The code shows that both ChangePack and ChangeID classes are auto-generated from protobuf definitions with proper handling of the versionVector field:

  • The field is correctly defined as an optional VersionVector type in both classes
  • Serialization/deserialization is handled automatically through the protobuf runtime
  • The field is included in the field definitions for both classes, ensuring proper encoding/decoding

No manual updates are needed as the protobuf compiler has generated all the necessary code to handle the new field correctly.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for instantiations of ChangePack and ChangeID without the versionVector field.

# Test: Find all instances where ChangePack is instantiated without the versionVector field. Expect: No matches.
ast-grep --pattern 'new ChangePack {
  documentKey: $_,
  checkpoint: $_,
  snapshot: $_,
  changes: $_,
  minSyncedTicket: $_,
  isRemoved: $_
}'

# Test: Find all instances where ChangeID is instantiated without the versionVector field. Expect: No matches.
ast-grep --pattern 'new ChangeID {
  clientSeq: $_,
  serverSeq: $_,
  lamport: $_,
  actorId: $_
}'

Length of output: 251


Script:

#!/bin/bash
# Let's try a different approach to find ChangePack and ChangeID usage patterns

# Search for ChangePack instantiations and related code
rg "ChangePack" -A 5 -B 5

# Search for ChangeID instantiations and related code
rg "ChangeID" -A 5 -B 5

# Search for any files that might handle serialization/deserialization of these types
rg -l "toJSON|fromJSON|serialize|deserialize"

Length of output: 11162


Script:

#!/bin/bash
# Let's check for any usage of ChangePack or ChangeID construction/handling in the codebase

# Look for any code that creates or processes ChangePack or ChangeID
rg -B 3 -A 3 "new (ChangePack|ChangeID)"

# Look for any code that handles versionVector
rg -B 3 -A 3 "versionVector"

# Look for any test files that might verify ChangePack or ChangeID behavior
fd -e test.ts -e spec.ts -x rg -l "ChangePack|ChangeID" {}

Length of output: 3608

@@ -142,7 +142,7 @@ export const listDocumentHistoriesAsync = createAppThunk(
return getPaginationData({
data: histories,
isForward,
previousID: previousSeq,
previousID: String(previousSeq),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider updating getPaginationData to handle bigint natively

While the String() conversion works correctly here, consider updating getPaginationData's interface to handle bigint directly in a future update. This would maintain type consistency throughout the pagination logic and avoid unnecessary type conversions.

 export const getPaginationData = <T extends {}>(params: {
   data: Array<T>;
   isForward: boolean;
-  previousID: string;
+  previousID: bigint;
   pageSize: number;
   reverse?: boolean;
 }): {

Committable suggestion was skipped due to low confidence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants